Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: scope type should be string instead of int #3337

Merged
merged 1 commit into from
Nov 3, 2022
Merged

fix: scope type should be string instead of int #3337

merged 1 commit into from
Nov 3, 2022

Conversation

SavvasMohito
Copy link
Contributor

@SavvasMohito SavvasMohito commented Nov 3, 2022

Problem description

Since the v2.0.1 update, the oauth2_token_response model has been renamed and updated to o_auth2_token_exchange. Along with that, an issue occurred where the scope field of the token response was expected to be of type int (for some reason) instead of string as it should be, based on RFC 6749 and previous versions of hydra SDK.

Root of the problem

As figured out by @aeneasr and @jonas-jonas in ory/sdk#223, this problem seems to be caused by handler.go#L827. As mentioned earlier, the scope field should be of type string instead of int, as seen in the proposed change of this PR.

Examples of the problem

These examples show how this change affected the generated python and go clients, but the same behaviour can be found in the rest of the clients. I compare the previous version 1.11.0, which worked with no issues, to the latest version 2.0.1, which cannot complete the token exchange flow successfully.

Python Client

Before (oauth2_token_response.py#L88)

return {
    'access_token': (str,),  # noqa: E501
    'expires_in': (int,),  # noqa: E501
    'id_token': (int,),  # noqa: E501
    'refresh_token': (str,),  # noqa: E501
--> 'scope': (str,),  # noqa: E501
    'token_type': (str,),  # noqa: E501
}

After (o_auth2_token_exchange.py#L89)

return {
    'access_token': (str,),  # noqa: E501
    'expires_in': (int,),  # noqa: E501
    'id_token': (int,),  # noqa: E501
    'refresh_token': (str,),  # noqa: E501
--> 'scope': (int,),  # noqa: E501
    'token_type': (str,),  # noqa: E501
}

Go Client

Before (model_oauth2_token_response.go#L23)

type Oauth2TokenResponse struct {
	AccessToken *string `json:"access_token,omitempty"`
	ExpiresIn *int64 `json:"expires_in,omitempty"`
	IdToken *string `json:"id_token,omitempty"`
	RefreshToken *string `json:"refresh_token,omitempty"`
    --> Scope *string `json:"scope,omitempty"`
	TokenType *string `json:"token_type,omitempty"`
}

After (model_o_auth2_token_exchange.go#L29)

type OAuth2TokenResponse struct {
	AccessToken *string `json:"access_token,omitempty"`
	ExpiresIn *int64 `json:"expires_in,omitempty"`
	IdToken *int64 `json:"id_token,omitempty"`
	RefreshToken *string `json:"refresh_token,omitempty"`
    --> Scope *int64 `json:"scope,omitempty"`
	TokenType *string `json:"token_type,omitempty"`
}

Related issue(s)

This PR came out from ory/sdk#220 and ory/sdk#223.

Importance of the solution

In my opinion, the proposed solution is of high importance. Right now, with the current version of the generated clients, the token change flow, which is the core function of this library, is not working. Hopefully, the suggested update is going to solve this type mismatch and bring the token flows back to normal.

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

@aeneasr aeneasr merged commit f59f1c6 into ory:master Nov 3, 2022
@aeneasr
Copy link
Member

aeneasr commented Nov 3, 2022

Awesome, thank you! 🎉 Your contribution makes Ory better :)

@vinckr
Copy link
Member

vinckr commented Nov 28, 2022

Hey @SavvasMohito
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants